Fix #8123 #8128
Fix #8123 #8128
Conversation
| @@ -11,7 +11,7 @@ Object.assign($.fn.bootstrapTable.defaults, { | |||
| showAutoRefresh: true, | |||
| autoRefreshInterval: 60, | |||
| autoRefreshSilent: true, | |||
| autoRefreshStatus: true, | |||
| autoRefreshStatus: false, | |||
There was a problem hiding this comment.
Should the default value here be modified?
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #8123 by correcting the auto-refresh extension's toggle behavior and default state. The auto-refresh button now starts in an inactive state by default and properly toggles its visual state when clicked.
Changes:
- Changed default
autoRefreshStatusfromtruetofalseto prevent auto-refresh from starting automatically - Fixed button selector from class-based to attribute-based to match the framework's button naming convention
- Refactored attribute handling to conditionally apply the active class based on refresh status
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/extensions/auto-refresh/bootstrap-table-auto-refresh.js | Updated default state, fixed button selectors, and improved attribute handling for the auto-refresh toggle button |
| cypress/e2e/extensions/auto-refresh/test.html | Added test HTML page for auto-refresh functionality testing |
| cypress/e2e/extensions/auto-refresh/auto-refresh.cy.js | Added Cypress test to verify toggle behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| attributes.class = this.constants.classes.buttonActive || 'active' | ||
| } | ||
|
|
||
| this.buttons = Object.assign(this.buttons || {}, { |
There was a problem hiding this comment.
The defensive || {} fallback is unnecessary here. The base class always initializes this.buttons in the initConstants() method (see src/modules/initialization.js line 32-34). This is inconsistent with other extensions like copy-rows, custom-view, export, filter-control, and toolbar which use Object.assign(this.buttons, {...}) without the fallback. While this doesn't cause issues, it's better to maintain consistency across the codebase.
| this.buttons = Object.assign(this.buttons || {}, { | |
| Object.assign(this.buttons, { |
| cy.visit('cypress/e2e/extensions/auto-refresh/test.html') | ||
| cy.get('[name="autoRefresh"]').click() |
There was a problem hiding this comment.
The test should verify the initial state of the auto-refresh button. Since the default value of autoRefreshStatus was changed to false, the button should not have the 'active' class when the page first loads. Consider adding an assertion before the first click to verify this initial state.
| 'aria-label': this.options.formatAutoRefresh(), | ||
| title: this.options.formatAutoRefresh() | ||
| } | ||
| event: this.toggleAutoRefresh.bind(this), |
There was a problem hiding this comment.
The .bind(this) call is redundant here. The toolbar module already calls event handlers with the correct context using .call(this) (see src/modules/toolbar.js line 230). While this doesn't cause any issues, it's unnecessary and inconsistent with other extensions like copy-rows, custom-view, and filter-control which don't use .bind().
| event: this.toggleAutoRefresh.bind(this), | |
| event: this.toggleAutoRefresh, |
🤔Type of Request
🔗Resolves an issue?
Fix #8123
📝Changelog
☑️Self Check before Merge